Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent duplicate HTTP headers when WriterInterceptor is used #27406

Merged
merged 1 commit into from
Aug 22, 2022

Conversation

geoand
Copy link
Contributor

@geoand geoand commented Aug 22, 2022

Fixes: #27325

Copy link
Contributor

@Sgitario Sgitario left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After digging into the code, I see that what this change is doing is avoiding calling ServerSerializers.encodeResponseHeaders (https://github.com/quarkusio/quarkus/blob/main/independent-projects/resteasy-reactive/server/runtime/src/main/java/org/jboss/resteasy/reactive/server/core/ServerSerialisers.java#L71).

This consumer is being invoked because it has been previously registered in https://github.com/quarkusio/quarkus/blob/main/independent-projects/resteasy-reactive/server/runtime/src/main/java/org/jboss/resteasy/reactive/server/core/ServerSerialisers.java#L194.

Therefore, I have two questions/comments:
a. Instead of setting the listener to null, why not avoid setting it in here: https://github.com/quarkusio/quarkus/blob/main/independent-projects/resteasy-reactive/server/runtime/src/main/java/org/jboss/resteasy/reactive/server/core/ServerSerialisers.java#L71
b. Or why not improving the logic of ServerSerializers.encodeResponseHeaders to remove the response header before adding it: vertxResponse.removeResponseHeader(header).

Note that I tried to do the option b. without removing the listener and it worked fine.

@geoand
Copy link
Contributor Author

geoand commented Aug 22, 2022

Very good questions indeed!

Below are my answers on why we can't do one or the other:

a. Instead of setting the listener to null, why not avoid setting it in here:

This code path is needed for all other cases other than the writer interceptor handling

b. Or why not improving the logic of ServerSerializers.encodeResponseHeaders to remove the response header before adding it

We don't want to do this because it's possible for Vert.x handlers that run before RESTEasy Reactive to add headers that are then written out when RESTEasy Reactive completes the processing

@Sgitario
Copy link
Contributor

Very good questions indeed!

Below are my answers on why we can't do one or the other:

a. Instead of setting the listener to null, why not avoid setting it in here:

This code path is needed for all other cases other than the writer interceptor handling

b. Or why not improving the logic of ServerSerializers.encodeResponseHeaders to remove the response header before adding it

We don't want to do this because it's possible for Vert.x handles that run before RESTEasy Reactive to add headers that are then written out

I don't fully understand the scenario of your last statement, but if you have already thought about these two solutions and decided to implement the current change, I trust your solution is the best approach :) Approving.

@geoand
Copy link
Contributor Author

geoand commented Aug 22, 2022

I don't fully understand the scenario of your last statement

The most common use case is this feature.

@geoand geoand added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Aug 22, 2022
@Sgitario
Copy link
Contributor

I don't fully understand the scenario of your last statement

The most common use case is this feature.

I've added a test in quarkus-resteasy-reactive/deployment to try to reproduce the scenario you described (using the property quarkus.http.header...). The test covers the same thing as the one you added in the independent project plus using the header property too.

The thing is that the test is behaving exactly the same either if you use the changes of this pull request or if you remove the header before setting it right before this line

.

This is the implementation:

package io.quarkus.resteasy.reactive.server.test.response;

import static io.restassured.RestAssured.when;
import static org.assertj.core.api.Assertions.assertThat;
import static org.hamcrest.Matchers.is;

import java.io.IOException;
import java.util.Arrays;
import java.util.Date;

import javax.ws.rs.GET;
import javax.ws.rs.Path;
import javax.ws.rs.Produces;
import javax.ws.rs.WebApplicationException;
import javax.ws.rs.container.ContainerRequestContext;
import javax.ws.rs.container.ContainerResponseContext;
import javax.ws.rs.container.ContainerResponseFilter;
import javax.ws.rs.core.MediaType;
import javax.ws.rs.core.Response;
import javax.ws.rs.ext.Provider;
import javax.ws.rs.ext.WriterInterceptor;
import javax.ws.rs.ext.WriterInterceptorContext;

import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.RegisterExtension;

import io.quarkus.test.QuarkusUnitTest;
import io.restassured.http.Headers;

public class CustomHeadersAndWriterInterceptorTest {

    @RegisterExtension
    static QuarkusUnitTest runner = new QuarkusUnitTest()
            .withApplicationRoot((jar) -> jar
                    .addClasses(DummyWriterInterceptor.class, TestResource.class))
            .overrideConfigKey("quarkus.http.header.\"propertyHeader\".value", "foo")
            .overrideConfigKey("quarkus.http.header.\"aggregateHeader\".value", "1");

    @Test
    void testResponseHeaders() {
        Headers headers = when()
                .get("/test")
                .then()
                .statusCode(200)
                .header("etag", is("0"))
                .extract().headers();
        assertThat(headers.getList("etag")).hasSize(1);
        assertThat(headers.getList("Last-Modified")).hasSize(1);
        assertThat(headers.getList("customHeader")).hasSize(1);
        assertThat(headers.getValue("customHeader")).isEqualTo("[a]");
        assertThat(headers.getValue("propertyHeader")).isEqualTo("foo");
        assertThat(headers.getList("aggregateHeader")).hasSize(2);
        assertThat(headers.getValue("aggregateHeader")).isEqualTo("[2]");
    }

    @Path("/test")
    public static class TestResource {

        @GET
        @Produces(MediaType.TEXT_PLAIN)
        public Response hello() {
            return Response.ok("123").lastModified(new Date()).header("etag", 0).build();
        }
    }

    @Provider
    public static class MyCustomHeaderFilter implements ContainerResponseFilter {

        @Override
        public void filter(ContainerRequestContext requestContext, ContainerResponseContext responseContext)
                throws IOException {
            responseContext.getHeaders().add("customHeader", Arrays.asList("a"));
            responseContext.getHeaders().add("aggregateHeader", Arrays.asList("2"));
        }
    }

    @Provider
    public static class DummyWriterInterceptor implements WriterInterceptor {

        @Override
        public void aroundWriteTo(WriterInterceptorContext context) throws IOException, WebApplicationException {
            context.proceed();
        }
    }

}

@geoand
Copy link
Contributor Author

geoand commented Aug 22, 2022

I might be misremembering when that Vert.x handler runs, but you should be able to reproduce the scenario I mentioned above if you create a custom Vert.x Web handler that runs before RESTEasy Reactive anb simply sets an HTTP header.

@geoand geoand merged commit e71c850 into quarkusio:main Aug 22, 2022
@quarkus-bot quarkus-bot bot added this to the 2.13 - main milestone Aug 22, 2022
@geoand geoand deleted the #27325 branch August 22, 2022 09:36
@quarkus-bot quarkus-bot bot added kind/bugfix and removed triage/waiting-for-ci Ready to merge when CI successfully finishes labels Aug 22, 2022
@gsmet gsmet modified the milestones: 2.13 - main, 2.12.0.Final Aug 23, 2022
@gsmet gsmet modified the milestones: 2.12.0.Final, 2.7.7.Final Jan 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Resteasy Reactive duplicates headers when custom WriterInterceptor is present
3 participants